Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

make parameter 'uplo' optional for [c]tricpy #24

Merged
merged 8 commits into from
Feb 20, 2024
Merged

make parameter 'uplo' optional for [c]tricpy #24

merged 8 commits into from
Feb 20, 2024

Conversation

jo-37
Copy link
Contributor

@jo-37 jo-37 commented Feb 8, 2024 •
edited
Loading

Enable calling $m->tricpy() to extract an upper triangular matrix as discussed at PerlMonks

This change will silently break code like

sequence(3,3,2)->tricpy(indx(1, 0))

@coveralls
Copy link

coveralls commented Feb 11, 2024 •
edited
Loading

Coverage Status

coverage: 42.475% (-0.06%) from 42.53%
when pulling 35dee59 on jo-37:contrib
into 965d247 on PDLPorters:master.

mohawk2
mohawk2 requested changes Feb 11, 2024
View reviewed changes
Copy link
Member

@mohawk2 mohawk2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this! It will break the code as you say, but I don't think there's much risk. I've done a quick search on grep.metacpan.org anyway.

Please could you make a couple of changes?

  • make the Doc be <<'EOT' and get rid of the \
  • not document ctricpy at all
  • update the tests to have hardcoded (maybe 3x3) values - you're literally using tricpy to test a version of tricpy, which is pointless

@jo-37
Copy link
Contributor Author

jo-37 commented Feb 11, 2024

Thanks for your review and your suggestions, fixed so far. However, I'm not sure what you mean with "not document ctricpy at all". Shall I completely remove its POD entry?

@mohawk2
Copy link
Member

mohawk2 commented Feb 12, 2024

Thanks for your review and your suggestions, fixed so far. However, I'm not sure what you mean with "not document ctricpy at all". Shall I completely remove its POD entry?

Yes please. As alluded on PerlMonks, anything that helps PDL::Complex is "legacy" and shouldn't be visibly supported (even though I'm not going to delete it just yet, and even if your nice new feature does quite rightly also apply to this legacy code).

@jo-37
Copy link
Contributor Author

jo-37 commented Feb 12, 2024

Moved the doc from POD to comment.

@mohawk2
Copy link
Member

mohawk2 commented Feb 13, 2024

It's fortunate that you did that, because it caused me to notice that you've put =for exampleS, not =for example. Sorry I missed that before. Please can you make it be spelled right for tricpy? You know how dang fussy computers can be.

I did ask you to remove the docs for ctricpy. Putting extra comments in the file makes for extra text that the maintainer (which is me) needs to mentally process whenever they need to make a change/read the code. If you would like to become the maintainer of this distro, go ahead and contribute a number of other things to it to the point that the PDL community has confidence you'll add value doing so. At that point, you can add as many comments as you like! Until then, please make the current maintainer's life easier by removing those comments.

@jo-37
Copy link
Contributor Author

jo-37 commented Feb 14, 2024

Removed the comment and fixed the format.

@mohawk2 mohawk2 merged commit a8599f8 into PDLPorters:master Feb 20, 2024
7 of 8 checks passed
@mohawk2
Copy link
Member

mohawk2 commented Feb 20, 2024

Thank you for your excellent contribution! Released as 0.38.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mohawk2 mohawk2 mohawk2 requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jo-37 @coveralls @mohawk2